Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

T352298: Add custom tooltip #111

Merged
merged 24 commits into from
Apr 29, 2024
Merged

T352298: Add custom tooltip #111

merged 24 commits into from
Apr 29, 2024

Conversation

medied
Copy link
Contributor

@medied medied commented Mar 27, 2024

https://phabricator.wikimedia.org/T352298

Use the Wordpress Popover component to fabricate our custom tooltip

src/link/edit.js Outdated
const toolbarButtonRef = useRef();
const [ displayCustomTooltip, setDisplayCustomTooltip ] = useState( false );
// eslint-disable-next-line no-undef
const customTooltipDisplayedCount = parseInt( localStorage.getItem( 'WikipediaPreviewWordpressPlugin-CustomTooltipDisplayedCount' ) ) || 0;
Copy link
Contributor Author

@medied medied Apr 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opting for localStorage here to save this count state, useState wouldn't work here as it gets reset on page reload. Yet the specification asks to re-display tooltip at least a second time:

Reattempt displaying the tooltip in the subsequent editing session to provide another opportunity for the user to view it. Cap the tooltip reattempts to just one additional session to prevent repetitive intrusion

Hence customTooltipDisplayedLimit which can be increased to 2 (or whatever limit). However this is what I would like to discuss with Sudhanshu, details of this UX, what should we look for here to decide whether to display tooltip again? For example, we could look into whether there are already previews in the post: if no previews added, display again; otherwise we could infer the user learned to add the preview as there are present already.

Also localStorage gets reset on a fresh/new browser session, which makes us run into the risk of the tooltip being annoying. Perhaps another reason to check whether previews are added already.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is showing up for editors within the admin interface, we can use a property for storage. Like we did for the review banner.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, here's one implementation: 2c8c1f4, do you think there's a better way?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm exploring with a propagated property here but looks like the global var only gets updated on page reload so it may not work for what we need

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

correct me if I miss the context here, this need to be documented well and easy to confuse if we revisit this logic again in few months/years later.

If the toolbar is closed before the 5-second display period, skip the tooltip for the remainder of the session.

This can handled by localStorage, so it checks through the current session

Once the tooltip has been displayed for the full duration, do not show it in future sessions to avoid redundancy.

full duration for 5 seconds, then we set post the incrementDisplayedCount

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm exploring with a propagated property here but looks like the global var only gets updated on page reload so it may not work for what we need

It's propagated from php to js on page load. After that, you have to update it yourself on the js side.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's propagated from php to js on page load. After that, you have to update it yourself on the js side.

Got it, updating directly on JS side by incrementing directly via window (and without useState): 62b3868

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

correct me if I miss the context here, this need to be documented well...

Yea this is part of the UX I would like to clarify with Sudhanshu, specially the other point:

Reattempt displaying the tooltip in the subsequent editing session to provide another opportunity for the user to view it. Cap the tooltip reattempts to just one additional session to prevent repetitive intrusion

I think we just need to first make sure we're on the same page of what a 'session' is in this case, and also just agree on when exactly we should display tooltip again. Let's circle back to this later

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just realise there is a sessionStorage https://developer.mozilla.org/en-US/docs/Web/API/Window/sessionStorage in browser, if we are dealing with session maybe this is what we can use.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right I came across that method too, but it's not really suited as we need the storage to be more permanent. From the docs:

...data in sessionStorage is cleared when the page session ends.

What we need is to be able to display the tooltip with the full 5 seconds duration, and then we don't show again.

@medied medied marked this pull request as ready for review April 3, 2024 18:25
src/link/edit.js Outdated Show resolved Hide resolved
src/link/edit.js Outdated Show resolved Hide resolved
src/link/tooltip.js Outdated Show resolved Hide resolved
src/link/edit.js Outdated Show resolved Hide resolved
src/link/style.scss Outdated Show resolved Hide resolved
src/link/edit.js Outdated Show resolved Hide resolved
@medied
Copy link
Contributor Author

medied commented Apr 18, 2024

Please see latest commits for the solution I'm proposing: instead of adding timers to keep track of exact milliseconds, we can just keep track of whether the tooltip was displayed for the whole five seconds or not. We only attempt to redisplay the tooltip if it hasn't been displayed in full duration and it's still under the display limit (2). Happy to share more of the reasoning if needed

@stephanebisson stephanebisson merged commit 6d49a85 into main Apr 29, 2024
1 check passed
@stephanebisson stephanebisson deleted the T352298-tooltip branch April 29, 2024 15:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants